Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rpc: faster getblockstats using BlockUndo data #14802

Merged
merged 1 commit into from
May 10, 2019

Conversation

FelixWeis
Copy link
Contributor

Using undo data for a block (rev?????.dat) we can retrieve value information about prevouts and calculate the final transaction fee (rate). This approach is about 80x faster, drops the requirement for -txindex, and works for all non-pruned blocks.

# 2018-11-25T16:36:19Z Bitcoin Core version v0.17.99.0-edc715240-dirty (release build)
seq 550100 550200  0.00s user 0.00s system 62% cpu 0.004 total
xargs -n1 src/bitcoin-cli getblockstats  0.21s user 0.19s system 17% cpu 2.302 total


# 2018-11-25T16:39:17Z Bitcoin Core version v0.17.0 (release build)
seq 550100 550200  0.00s user 0.00s system 87% cpu 0.002 total
xargs -n1 src/bitcoin-cli getblockstats  0.24s user 0.22s system 0% cpu 3:19.42 total

@gmaxwell
Copy link
Contributor

<3 I like this direction, certainly having a RPC that requires txindex is unfortunate, as does having rpcs that don't work (well) with pruning. I don't think it would have been worth it to store extra data to make stats fast, but since we already are...

Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK

@@ -128,8 +113,8 @@ def run_test(self):
assert_equal(stats_by_hash, self.expected_stats[i])

# Check with the node that has no txindex
Copy link
Member

@maflcko maflcko Nov 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The getblockstats code seems to no longer call into any txindex code (not even as fallback), so might as well remove the txindexed node from the whole test?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test and testdata changed

@DrahtBot
Copy link
Contributor

DrahtBot commented Nov 25, 2018

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

src/validation.cpp Outdated Show resolved Hide resolved
Copy link
Member

@promag promag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great change!

Mind adding a release note?

src/validation.h Outdated Show resolved Hide resolved
@jonasschnelli
Copy link
Contributor

Clever idea!
Concept ACK

@ajtowns
Copy link
Contributor

ajtowns commented Jan 2, 2019

ConceptACK, code looks good at first glance

@laanwj
Copy link
Member

laanwj commented Jan 22, 2019

Good to remove reliance on the transaction index.
utACK 602dd54

src/validation.h Outdated Show resolved Hide resolved
Copy link
Contributor

@marcinja marcinja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 76fef26

Nice work!

src/rpc/blockchain.cpp Outdated Show resolved Hide resolved
doc/release-notes.md Outdated Show resolved Hide resolved
@@ -1987,6 +1982,7 @@ static UniValue getblockstats(const JSONRPCRequest& request)
maxfeerate = std::max(maxfeerate, feerate);
minfeerate = std::min(minfeerate, feerate);
}
tx_n++;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks fragile. (If some paths in the for loop continue, this would be skipped)

@@ -1913,6 +1913,7 @@ static UniValue getblockstats(const JSONRPCRequest& request)
std::vector<std::pair<CAmount, int64_t>> feerate_array;
std::vector<int64_t> txsize_array;

size_t tx_n = 1;
for (const auto& tx : block.vtx) {
Copy link
Member

@maflcko maflcko Mar 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style-nit:

Suggested change
for (const auto& tx : block.vtx) {
for (size_t i = 0; i < block.vtx.size(); ++i) {
const auto& tx = block.vtx.at(i);
...// after coinbase check
const auto& txundo = blockUndo.vtxundo.at(i + 1);

maflcko pushed a commit that referenced this pull request Mar 20, 2019
fa11c03 refactor: Expose UndoReadFromDisk in header (MarcoFalke)

Pull request description:

  It is not possible to calculate the fee of a non-mempool transaction in RPCs unless txindex is active or the prevtxs are passed in through the RPC.

  Fix that issue for confirmed txs by exposing `UndoReadFromDisk` in the header file.

  This pull is a requirement for
  * rpc: faster getblockstats using BlockUndo data #14802
  *  Index for BIP 157 block filters #14121
  * my local patches

Tree-SHA512: 859ea5f2dfb4feac612b50faeb0e2b6c07b83f1d983e519d7647a78058d85c0390fd09ec66b460ae7a4c3b273e81b0013ee9f4bb8dfba0c4782ffaa1fa453ea6
@maflcko
Copy link
Member

maflcko commented Mar 25, 2019

@FelixWeis Are you still working on this?

@jonasschnelli
Copy link
Contributor

Don't let this go down unmaintained... please pick this up @FelixWeis

FelixWeis added a commit to FelixWeis/bitcoin that referenced this pull request Apr 24, 2019
src/rpc/blockchain.cpp Outdated Show resolved Hide resolved
@FelixWeis FelixWeis force-pushed the 201811_blockstats_undoblock branch 2 times, most recently from 93a2573 to a565355 Compare April 29, 2019 12:36
src/rpc/blockchain.cpp Outdated Show resolved Hide resolved
@maflcko
Copy link
Member

maflcko commented May 3, 2019

@FelixWeis You don't actually need to rebase. If you address my style-nits, the conflict will solve itself.

@FelixWeis FelixWeis force-pushed the 201811_blockstats_undoblock branch from 293cb0e to e7f0aea Compare May 9, 2019 23:04
@FelixWeis FelixWeis force-pushed the 201811_blockstats_undoblock branch 2 times, most recently from ca7cd73 to 92cd51e Compare May 9, 2019 23:33
@maflcko
Copy link
Member

maflcko commented May 10, 2019

Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

@FelixWeis FelixWeis force-pushed the 201811_blockstats_undoblock branch from 92cd51e to 8430f86 Compare May 10, 2019 12:23
Using undo data for a block (rev?????.dat) we can retrieve value information about prevouts and calculate the final transaction fee (rate). This approach is about 80x faster, drops the requirement for -txindex, and works for all non-pruned blocks.
@FelixWeis FelixWeis force-pushed the 201811_blockstats_undoblock branch from 8430f86 to d20d756 Compare May 10, 2019 12:34
@maflcko
Copy link
Member

maflcko commented May 10, 2019

re-utACK d20d756

Only changes are:

  • Make tests more deterministic
  • Add and use GetUndoChecked
  • Minor stylistic changes
Show signature and timestamp

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

re-utACK d20d7567528e216badb8475df298bb3cec008985

Only changes are:
* Make tests more deterministic
* Add and use GetUndoChecked
* Minor stylistic changes
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUh/4QwAwLZrUZJqLo1ctjMWO2SQ+ut4rxMDYaMNSIGOxw61u8J6OIUPsVfKBJLF
s2Gi0Q7XwCwlttNVDARBCQFH4R8IhVXLaY//BCfVPamVwXCO3s1x7GDtLZZrjs2c
Rng9VOhCSZ7wXZmgoQmYwj77pQOU2k1B6FXkMylzf/pt+W/jaANCuGtC4k4k8x/l
sM2sH75nWs2URra42exUbmDoIfohOHWfAxcA0Ni31AWkeRYRazWGmsvA93qL6PtP
/wN5rMNi10gCI83/N16ZZ+iq7djqRoCjkbkqSNd2D0NJOmquEIN4W3GGQJ1BciGF
nu0gLQ12ShXCVf167E6s4xGPsKu9hsiUNJN9xursiD1RUhGtUQXdpIRjodL+lDj8
CZhHeUj0whAXKkS+YddhPO385rzqChebvdWZL5stRlUi4TFKzJM+rvsjGNaEKgxO
wjiUfi1L6v07zstlKFLc1pfri9Yws5b4MT6fO9+9dQrtUm1IcS4NeS4YrKKaiI4X
B9R6OHC3
=xoBb
-----END PGP SIGNATURE-----

Timestamp of file with hash 5dc2003a827858f9302ac84ff924bdf15ada47e85278dce1eee93d098c8e8abd -

@maflcko maflcko merged commit d20d756 into bitcoin:master May 10, 2019
maflcko pushed a commit that referenced this pull request May 10, 2019
d20d756 rpc: faster getblockstats using BlockUndo data (Felix Weis)

Pull request description:

  Using undo data for a block (rev?????.dat) we can retrieve value information about prevouts and calculate the final transaction fee (rate). This approach is about 80x faster, drops the requirement for `-txindex`, and works for all non-pruned blocks.

  ```
  # 2018-11-25T16:36:19Z Bitcoin Core version v0.17.99.0-edc715240-dirty (release build)
  seq 550100 550200  0.00s user 0.00s system 62% cpu 0.004 total
  xargs -n1 src/bitcoin-cli getblockstats  0.21s user 0.19s system 17% cpu 2.302 total

  # 2018-11-25T16:39:17Z Bitcoin Core version v0.17.0 (release build)
  seq 550100 550200  0.00s user 0.00s system 87% cpu 0.002 total
  xargs -n1 src/bitcoin-cli getblockstats  0.24s user 0.22s system 0% cpu 3:19.42 total
  ```

ACKs for commit d20d75:
  MarcoFalke:
    re-utACK d20d756

Tree-SHA512: 5babc3eb8d2fee2cb23dc12f522656b80737a540cbf2b13390a8f388304c46c064cca76f896b46a6e2abae8cc582d28e1ab20dd4bb17ad6142f20630c2d30c54
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Sep 3, 2019
Using undo data for a block (rev?????.dat) we can retrieve value information about prevouts and calculate the final transaction fee (rate). This approach is about 80x faster, drops the requirement for -txindex, and works for all non-pruned blocks.

Github-Pull: bitcoin#14802
Rebased-From: d20d756 (diff minimised)
deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Jun 2, 2020
Summary:
Using undo data for a block (rev?????.dat) we can retrieve value information about prevouts and calculate the final transaction fee (rate). This approach is about 80x faster, drops the requirement for -txindex, and works for all non-pruned blocks.

bitcoin/bitcoin@d20d756

---

Depends on D6304

Backport of Core [[bitcoin/bitcoin#14802 | PR14802]]

Test Plan:
  ninja
  ./test/functional/test_runner.py rpc_getblockstats --gen-test-data
  ninja check-all

run bitcoin-cli getblockstats `non-pruned-block-height` on a node without -txindex now works

Reviewers: #bitcoin_abc, deadalnix

Reviewed By: #bitcoin_abc, deadalnix

Subscribers: deadalnix

Differential Revision: https://reviews.bitcoinabc.org/D6305
ftrader added a commit to bitcoin-cash-node/bitcoin-cash-node that referenced this pull request Dec 1, 2020
RPC: Improve performance of `getblockstats` & `getblock`

See merge request bitcoin-cash-node/bitcoin-cash-node!849

This description below taken from !849 as instructed (with minor cleanups).
Thanks to Calin Culianu for bringing it all together.

The authors of this merge request were:

Co-Authored-By: Calin Culianu <calin.culianu@gmail.com>
Co-Authored-By: Axel Gembe <derago@gmail.com>

For the included parts of backport of bitcoin/bitcoin#14802:
Co-Authored-By: Felix Weis <mail@felixweis.com>
Co-Authored-By: MarcoFalke <falke.marco@gmail.com>

Summary
-------

It was observed during the recent round of ScaleNet tests by @ago that
the `getblockstats` call is very slow.  This call is used by block
explorers and as such the extant block explorers were timing out when
issuing `getblockstats` on large ScaleNet blocks.

`getblockstats` has a bunch of problems:

- It keeps `cs_main` held the entire time, which is not necessary.
- This is particularly problematic because it must iterate over all the
  inputs in a block, reading their ancestor tx's from disk. This
  operation can take tens of seconds to minutes on very large 300k input
  ScaleNet blocks.
- During this time `cs_main` is held and **the entire node freezes**.
- What's more, it would potentially read the same tx multiple times from
  disk as it iterated over the inputs.
- It also had a bunch of un-optimized UniValue API usage (this is
  minor).

What's changed:

- Release `cs_main` as soon as we don't need it anymore. It's only
  needed to look-up the block. Once we have the `CBlockIndex`, we don't
  need it anymore.  All of the rest of the code was reviewed carefully and
  is lock-free.
- Don't call `GetTransaction()` because it grabs `cs_main` and also is very slow
  and redundant. We instead backported part of Core#14802, and we use the
  undo (.rev) file to get previous inputs in order to calculate fees.
- Optimized the UniValue usage to our new (faster, safer) API.
- Leverage NRVO/RVO in C++ by returning only 1 thing in 1 branch (this
  is a minor optimizaton compared to all the others).

In addition, we modified `getblock` to release the lock earlier (was MR !850):

- We refactored `GetBlockChecked` (called in only 2 places, `getblock` and `getblockstats`)
  out into 2 functions: `ThrowIfPrunedBlock` (requires `cs_main`) and the slower `ReadBlockChecked` 
  (does *not* require locks). We also added the new `ReadUndoChecked` for reading the inputs from 
  the undo file.

Sorry for having 2 commits in 1 MR but the refactor was necessary, since to do it right we
needed to touch `getblock`.


Test Plan
---------

- Basic: `ninja check check-functional`
- Advanced: We want to illustrate the speedup here, so you will need
  ScaleNet for best results.
  1. Synch up a ScaleNet node, be sure to enable RPC on it in the conf
     file or via the right CLI arg magic. Also **enable** `txindex=1`
     (you will need this only for testing the version of the client without this MR)
  2. Create a shell script similar to the below, call it `cmd.sh`:

```bash
#!/bin/bash

blocks="16029 16026 16037 16130 16179 15813 16124"

for ht in $blocks; do
    src/bitcoin-cli -conf=$HOME/bitcoin/bchn/scalenet.conf getblockstats $ht &
done

wait
```

 3. **Note:** Be sure to replace the `-conf` arg in the shell script above with
    the conf file you created for your scalenet node.
 4. Next, build `bitcoind` with this commit and start the node, wait for it to
    synch fully and settle.
 5. Execute the following: `time ./cmd.sh`  (if you get an error about
    txindex, this is normal and unrelated to this commit. Wait a bit for it to
    catch up and try again).
 6. You should see it spit out a bunch of JSON results and a time.
 7. Do 5-6 above again but this time with a `bitcoind` built against
    master (without this commit).  Note how the time is multiple times longer,
    or more, depending on your machine.

This commit enhance the node to no longer need `txindex` for `getblockstats`,
otherwise correctness of the `getblockstats` call is identical, and now more
efficient.

Special thanks to `Axel Gembe <derago@gmail.com>` for his hard work in
troubleshooting and investigating this.  This code is largely inspired
by his test code here: https://gitlab.com/ago/bitcoin-cash-node/-/tree/optimize_getblockstats

Also special thanks to `Mark Lundeberg <5505128-markblundeberg@users.noreply.gitlab.com>`
for his tip on using the latest core changes that use the undo file rather
than the (slower) txindex lookups per input.
maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Dec 24, 2020
…data

66d012a test: RPC: getblock fee calculations (Elliott Jin)
bf7d6e3 RPC: getblock: tx fee calculation for verbosity 2 via Undo data (Elliott Jin)

Pull request description:

  This change is progress towards bitcoin#18771 .  It adapts the fee calculation part of bitcoin#16083 and addresses some feedback.  The additional "verbosity level 3" features are planned for a future PR.

  **Original PR description:**

  > Using block undo data (like in bitcoin#14802) we can now show fee information for each transaction in a block without the need for additional -txindex and/or a ton of costly lookups. For a start we'll add transaction fee information to getblock verbosity level 2. This comes at a negligible speed penalty (<1%).

ACKs for top commit:
  luke-jr:
    tACK 66d012a
  fjahr:
    tACK 66d012a
  MarcoFalke:
    review ACK 66d012a 🗜

Tree-SHA512: be1fe4b866946a8dc36427f7dc72a20e10860e320a28fa49bc85bd2a93a0d699768179be29fa52e18b2ed8505d3ec272e586753ef2239b4230e0aefd233acaa2
vijaydasmp pushed a commit to vijaydasmp/dash that referenced this pull request Dec 11, 2021
d20d756 rpc: faster getblockstats using BlockUndo data (Felix Weis)

Pull request description:

  Using undo data for a block (rev?????.dat) we can retrieve value information about prevouts and calculate the final transaction fee (rate). This approach is about 80x faster, drops the requirement for `-txindex`, and works for all non-pruned blocks.

  ```
  # 2018-11-25T16:36:19Z Bitcoin Core version v0.17.99.0-edc715240-dirty (release build)
  seq 550100 550200  0.00s user 0.00s system 62% cpu 0.004 total
  xargs -n1 src/bitcoin-cli getblockstats  0.21s user 0.19s system 17% cpu 2.302 total

  # 2018-11-25T16:39:17Z Bitcoin Core version v0.17.0 (release build)
  seq 550100 550200  0.00s user 0.00s system 87% cpu 0.002 total
  xargs -n1 src/bitcoin-cli getblockstats  0.24s user 0.22s system 0% cpu 3:19.42 total
  ```

ACKs for commit d20d75:
  MarcoFalke:
    re-utACK d20d756

Tree-SHA512: 5babc3eb8d2fee2cb23dc12f522656b80737a540cbf2b13390a8f388304c46c064cca76f896b46a6e2abae8cc582d28e1ab20dd4bb17ad6142f20630c2d30c54
vijaydasmp pushed a commit to vijaydasmp/dash that referenced this pull request Dec 13, 2021
d20d756 rpc: faster getblockstats using BlockUndo data (Felix Weis)

Pull request description:

  Using undo data for a block (rev?????.dat) we can retrieve value information about prevouts and calculate the final transaction fee (rate). This approach is about 80x faster, drops the requirement for `-txindex`, and works for all non-pruned blocks.

  ```
  # 2018-11-25T16:36:19Z Bitcoin Core version v0.17.99.0-edc715240-dirty (release build)
  seq 550100 550200  0.00s user 0.00s system 62% cpu 0.004 total
  xargs -n1 src/bitcoin-cli getblockstats  0.21s user 0.19s system 17% cpu 2.302 total

  # 2018-11-25T16:39:17Z Bitcoin Core version v0.17.0 (release build)
  seq 550100 550200  0.00s user 0.00s system 87% cpu 0.002 total
  xargs -n1 src/bitcoin-cli getblockstats  0.24s user 0.22s system 0% cpu 3:19.42 total
  ```

ACKs for commit d20d75:
  MarcoFalke:
    re-utACK d20d756

Tree-SHA512: 5babc3eb8d2fee2cb23dc12f522656b80737a540cbf2b13390a8f388304c46c064cca76f896b46a6e2abae8cc582d28e1ab20dd4bb17ad6142f20630c2d30c54
vijaydasmp pushed a commit to vijaydasmp/dash that referenced this pull request Dec 14, 2021
d20d756 rpc: faster getblockstats using BlockUndo data (Felix Weis)

Pull request description:

  Using undo data for a block (rev?????.dat) we can retrieve value information about prevouts and calculate the final transaction fee (rate). This approach is about 80x faster, drops the requirement for `-txindex`, and works for all non-pruned blocks.

  ```
  # 2018-11-25T16:36:19Z Bitcoin Core version v0.17.99.0-edc715240-dirty (release build)
  seq 550100 550200  0.00s user 0.00s system 62% cpu 0.004 total
  xargs -n1 src/bitcoin-cli getblockstats  0.21s user 0.19s system 17% cpu 2.302 total

  # 2018-11-25T16:39:17Z Bitcoin Core version v0.17.0 (release build)
  seq 550100 550200  0.00s user 0.00s system 87% cpu 0.002 total
  xargs -n1 src/bitcoin-cli getblockstats  0.24s user 0.22s system 0% cpu 3:19.42 total
  ```

ACKs for commit d20d75:
  MarcoFalke:
    re-utACK d20d756

Tree-SHA512: 5babc3eb8d2fee2cb23dc12f522656b80737a540cbf2b13390a8f388304c46c064cca76f896b46a6e2abae8cc582d28e1ab20dd4bb17ad6142f20630c2d30c54
vijaydasmp pushed a commit to vijaydasmp/dash that referenced this pull request Dec 14, 2021
d20d756 rpc: faster getblockstats using BlockUndo data (Felix Weis)

Pull request description:

  Using undo data for a block (rev?????.dat) we can retrieve value information about prevouts and calculate the final transaction fee (rate). This approach is about 80x faster, drops the requirement for `-txindex`, and works for all non-pruned blocks.

  ```
  # 2018-11-25T16:36:19Z Bitcoin Core version v0.17.99.0-edc715240-dirty (release build)
  seq 550100 550200  0.00s user 0.00s system 62% cpu 0.004 total
  xargs -n1 src/bitcoin-cli getblockstats  0.21s user 0.19s system 17% cpu 2.302 total

  # 2018-11-25T16:39:17Z Bitcoin Core version v0.17.0 (release build)
  seq 550100 550200  0.00s user 0.00s system 87% cpu 0.002 total
  xargs -n1 src/bitcoin-cli getblockstats  0.24s user 0.22s system 0% cpu 3:19.42 total
  ```

ACKs for commit d20d75:
  MarcoFalke:
    re-utACK d20d756

Tree-SHA512: 5babc3eb8d2fee2cb23dc12f522656b80737a540cbf2b13390a8f388304c46c064cca76f896b46a6e2abae8cc582d28e1ab20dd4bb17ad6142f20630c2d30c54
Munkybooty pushed a commit to Munkybooty/dash that referenced this pull request Feb 6, 2022
d20d756 rpc: faster getblockstats using BlockUndo data (Felix Weis)

Pull request description:

  Using undo data for a block (rev?????.dat) we can retrieve value information about prevouts and calculate the final transaction fee (rate). This approach is about 80x faster, drops the requirement for `-txindex`, and works for all non-pruned blocks.

  ```
  # 2018-11-25T16:36:19Z Bitcoin Core version v0.17.99.0-edc715240-dirty (release build)
  seq 550100 550200  0.00s user 0.00s system 62% cpu 0.004 total
  xargs -n1 src/bitcoin-cli getblockstats  0.21s user 0.19s system 17% cpu 2.302 total

  # 2018-11-25T16:39:17Z Bitcoin Core version v0.17.0 (release build)
  seq 550100 550200  0.00s user 0.00s system 87% cpu 0.002 total
  xargs -n1 src/bitcoin-cli getblockstats  0.24s user 0.22s system 0% cpu 3:19.42 total
  ```

ACKs for commit d20d75:
  MarcoFalke:
    re-utACK d20d756

Tree-SHA512: 5babc3eb8d2fee2cb23dc12f522656b80737a540cbf2b13390a8f388304c46c064cca76f896b46a6e2abae8cc582d28e1ab20dd4bb17ad6142f20630c2d30c54
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants